Support for non-CL Clang on Windows by HJfod · Pull Request #981 · quickjs-ng/quickjs

HJfod · 2025-03-16T22:51:10Z

Clang on Windows without MSVC command line is a little bit quirky, so this PR adds support for it. I had an earlier PR (#886) for this too, but closed that one as I got too busy at the time to finish it. Now this works for the whole codebase as well as has CI tests for the new architechture combination.

Clang on Windows defines _MSC_VER, but it doesn't understand _Printf_format_string_

It doesn't generate separate Debug and Release directories and instead puts the executable directly under build/

bnoordhuis

Mostly LGTM but I believe @saghul reviewed this the first time around so he's likely more qualified to sign off on it.

bnoordhuis · 2025-03-17T09:39:14Z

CMakeLists.txt

+if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)
+add_compile_definitions(_CRT_SECURE_NO_WARNINGS)
+add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)
+endif()


Suggested change

if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)

add_compile_definitions(_CRT_SECURE_NO_WARNINGS)

add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)

endif()

if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)

add_compile_definitions(_CRT_SECURE_NO_WARNINGS)

add_compile_definitions(_CRT_NONSTDC_NO_DEPRECATE)

endif()

this did not seem to be there or working if the branch was current yesterday. I had to manual #define and also "#pragma warning(disable : 4996)" in several files for MS Clang to compile.

forget my comment ... I upgraded and compile now with msbuild and the problem is fixed.

bnoordhuis · 2025-03-17T09:44:25Z

quickjs-libc.c

 }

-#ifdef __GNUC__
+#if defined(__GNUC__) || defined(__clang__)


Doesn't clang-on-windows define __GNUC__? Because clang-on-linux definitely does:

$ clang -E -dM - < /dev/null | grep GNUC #define __GNUC_MINOR__ 2 #define __GNUC_PATCHLEVEL__ 1 #define __GNUC_STDC_INLINE__ 1 #define __GNUC__ 4

I believe it does too. What are you trying to fix here @HJfod ?

Removing the defined(__clang__) check makes it not work, so seems like Clang doesn't define __GNUC__

saghul · 2025-03-17T09:56:50Z

.github/workflows/ci.yml

          cl.exe /DJS_NAN_BOXING=1 /Zs cxxtest.cc

-  windows-clang:
+  windows-ninja-clang:


Nit, can we call this windows-real-clang perhaps?

saghul · 2025-03-17T09:57:52Z

CMakeLists.txt


+# Clang on Windows without MSVC command line fails because the codebase uses 
+# functions like strcpy over strcpy_s
+if(CMAKE_C_COMPILER_ID STREQUAL "Clang" AND WIN32)


Don't you also want to add NOT MSVC here, so make sure it's not ClangCL ?

saghul · 2025-03-17T09:58:20Z

CMakeLists.txt

    if(MSVC)
        set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:8388608")
-    else()
+    elseif(MINGW)


Why do you want to skip this on Clang?

Clang didn't seem to understand the parameter syntax and I couldn't find an equivalent option. Can take another look 👍

--stack_size
Clang in MS environment doesn't have a clang-gcc program, it has a clang-cpp program. no it's not GCC. In fact It does not carry the MSVC flag, it carries the WIN32 flag but MSYS2 Clang carries WIN32 as well. I don't think there's a special flag.
so what I did was if (msvc) elseif (GNU) else () .. doesn't seem to see Clang on my end!
Correction:. Clang knows nothing about GCC or GNU. in MSYS2 Clang responds to MINGW.
There may be something intermittent about how Clang responds to flags. I ended up reinstalling my build systems more than once, trying to reproduce a bug; if Ninja is installed a certain way in MSYS2 (the way that WinLibs does it). I verified that Clang will lose the WIN32 flag.

I was wrong. --stack_size is clang on mac. there's no stack size in MS version and I don't think there is in linux. What it does do is ignore the flag --stack_size and not --stack, and I get 140 warnings and don't see it.
oh and just fyi .. I reinstalled my build environments for clang and the flags completely change between ninja, make, and msbuild. so that section acts differently. I've seen 4 different flagsets today. but I have no 'Clang' flag in MSYS or MSVC. Clang in the MINGW environment gives no WIN32 flag. if someone uses WinLibs, I bet this flag isn't even there.

cutils.h

saghul · 2025-03-17T10:00:01Z

quickjs-libc.c

 }

-#ifdef __GNUC__
+#if defined(__GNUC__) || defined(__clang__)


I believe it does too. What are you trying to fix here @HJfod ?

saghul · 2025-03-17T10:01:35Z

getopt_compat.h

 #include <stdarg.h>
 #include <stdio.h>
 #include <windows.h>
+/* Required for JS_PRINTF_FORMAT */


Can we not do this and silence the warnings if need be? I actually want to get rid of this header altogether by parsing command line arguments by hand in qjsc, since it's only used there...

quickjs.h

* CI action renamed to just `windows-clang` * Remove inclusion of `cutils.h` from `getopt_compat.h` and just suppress format string literal warnings * Add more comments to `quickjs-libc.c` and `quickjs.h` * Update `CMakeLists.txt` to also set 8 MB stack size for Windows Clang

saghul · 2025-04-08T11:11:57Z

Can you please rebase now that getopt_compat is gone?

saghul · 2025-04-15T07:39:07Z

@HJfod Ping?

HJfod · 2025-05-04T19:26:37Z

Oh sorry, didn't notice this! I merged master to my fork and it compiled succesfully (although the docs action seems to have failed). Will also be testing it in a bit!

saghul · 2025-05-09T07:49:25Z

Nice!

HJfod added 13 commits February 5, 2025 22:45

Fixes to format preprocessing for Clang on Windows

c7a9a92

Clang on Windows defines _MSC_VER, but it doesn't understand _Printf_format_string_

Add Clang & Ninja workflow on Windows

2b1f713

Fix Windows Clang + Ninja action

51f5332

Add JS_PRINTF_FORMAT to getopt_compat

1607d47

Add missing JS_PRINTF_FORMAT

bb6dfe0

Fix warnings in quickjs-libc.c & update printf macro in quickjs.h

6731454

Merge branch 'master' of https://github.com/quickjs-ng/quickjs

3073f11

Fix Windows Clang build by suppressing CRT warnings

deb2196

Fix building with non-CL Clang on Windows

f08401e

It doesn't generate separate Debug and Release directories and instead puts the executable directly under build/

More fixes to Windows Clang workflow

de85fe6

Fix outdated name of QJS_BUILD_EXAMPLES option in Windows Clang

0ccbeae

Remove outdated interrupt test from Windows Clang action

9529a1b

Merge branch 'master' of https://github.com/quickjs-ng/quickjs

e311961

bnoordhuis reviewed Mar 17, 2025

View reviewed changes

saghul reviewed Mar 17, 2025

View reviewed changes

saghul approved these changes Mar 18, 2025

View reviewed changes

Merge branch 'master' of https://github.com/quickjs-ng/quickjs

ebee0ce

saghul approved these changes May 9, 2025

View reviewed changes

Conversation

HJfod commented Mar 16, 2025

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saghul commented Apr 8, 2025

Uh oh!

saghul commented Apr 15, 2025

Uh oh!

HJfod commented May 4, 2025

Uh oh!

saghul commented May 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coreyapowell May 29, 2025 •

edited

Loading

coreyapowell May 30, 2025 •

edited

Loading